Create Global Context #256
Conversation
|
@demiankatz I've moved the ThemeMenu component and associated global state to the dark mode PR (#258). |
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @crhallberg, this is definitely moving in the right direction, but there are some bits that don't seem to be fully fleshed out yet (you probably already know that :-) ). Hopefully the observations below will help to move things along. I'd also do a global search for "toggle" in case there are other modal-related calls that haven't been done yet. I was expecting to see more than I did while reviewing, but maybe I just overestimated what to expect. :-)
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
@crhallberg, just one small suggestion here based on recent changes. Beyond that, what's the state of this project? I see we still have failing tests. How can I help? Do you want me to help work on any automated or hands-on testing, or should I just give you more time to continue this? :-)
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
@crhallberg, I've done another manual review of this code following yesterday's work. I think this is in very good shape, but I had a few minor suggestions and discussion points. I'm happy to handle some or all of the work, depending on your preferences. Once all of this is sorted out, I'll do some hands-on testing to assure myself it's all good, but I don't expect any problems that should block a merge.
One other general comment: I did carefully inspect all the snapshots generated by the snapshot tests, and there were no unexpected/unwanted changes there. The trick to making those diffs readable is to add ?w=1 to the GitHub URL to ignore whitespace.
| const clickAction = () => { | ||
| setStateModalActivePid(pid); | ||
| toggleStateModal(); | ||
| toggleModal("state"); |
There was a problem hiding this comment.
Should this be openModal for consistency with other components, or is there a reason to use toggleModal?
| open={isDatastreamModalOpen} | ||
| onClose={toggleDatastreamModal} | ||
| open={isModalOpen("datastream")} | ||
| onClose={() => closeModal("datastream")} |
There was a problem hiding this comment.
In other components, we define a local closeXModal function instead of using the () => ... syntax. Both approaches are valid, but I wonder if we should choose one or the other for consistency. I don't feel a lot of pressure to change anything, but let me know if you have an opinion!
There was a problem hiding this comment.
I think we can leave this alone for now.
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
@crhallberg, thanks for the progress here! Just one new comment (which may not require any action). Please also see my two existing comments from May, 2023 and let me know if you want to do anything with those. I suspect we can leave the reported inconsistency alone, but I think it may make sense to replace the "toggle" with "open" to be more specific. Once all this is sorted, I think we can merge!
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
I believe that our work here is done!
Uh oh!
There was an error while loading. Please reload this page.